-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix gocognit output when a function has generic receiver type #10
Conversation
Do you have a file to test? |
Yes, below is my testing file. I realised that I wasn't handling the scenario when the receiver has multiple type params. I've implemented the missing piece. I can close this PR and open another one with single commit if you'd like. package main
type Node[T any] struct {
}
func (n *Node[T]) String() string {
if n != nil {
return "Node"
}
return ""
}
type Pair[K any, V any] struct {
Key K
Value V
}
func (p *Pair[K, V]) String() string {
if p != nil {
return "Pair"
}
return ""
}
type Triple[K any, V any, T any] struct {
}
func (t *Triple[K, V, T]) String() string {
if t != nil {
return "Triple"
}
return ""
}
func main() {
} |
You can modify the existing PR or create the new one. |
It's a single commit now. |
Hi @ivanruski, thanks for the awesome work. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also need to make sure this will compatible previous golang version. At least start from 1.16
gocognit.go
Outdated
case *ast.IndexListExpr: | ||
targs := make([]string, 0, len(t.Indices)) | ||
for _, exp := range t.Indices { | ||
targs = append(targs, recvString(exp)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The targs
slice was created using make
with the non-zero length. Why do we still use append
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am using the second version of make
for creating slices, where the second argument is the length of the slice and the third argument is the capacity of the underlying array. In this case go won't resize the underlying array, because I specified enough capacity. https://go.dev/play/p/rZmxsFAvmF1.
I can change it to:
targs := make([]string, len(t.Indices))
for i, exp := range t.Indices {
targs[i] = recvString(exp)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are absolutely right. The code works.
Btw, it seems that using assignments is faster (most of the time). What do you think?
see: https://github.com/uudashr/go-bench and try to run
$ go test -benchmem -run="^$" -bench="^BenchmarkSlice_PreAllocated"
goos: darwin
goarch: arm64
pkg: github.com/uudashr/go-bench
BenchmarkSlice_PreAllocated_Assign-8 1000000000 0.0000005 ns/op 0 B/op 0 allocs/op
BenchmarkSlice_PreAllocated_Append-8 1000000000 0.0000010 ns/op 0 B/op 0 allocs/op
PASS
ok github.com/uudashr/go-bench 0.110s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I observe the same thing (that assigning is faster). I will changed it.
goos: darwin
goarch: arm64
pkg: github.com/uudashr/go-bench
BenchmarkSlice_PreAllocated_Assign-10 1000000000 0.0000004 ns/op
BenchmarkSlice_PreAllocated_Append-10 1000000000 0.0000006 ns/op
PASS
ok github.com/uudashr/go-bench 0.239s
Hi @uudashr, good catch I've tested it only with go1.18 and missed that. I am mentioning this because I am not sure how to make it work for 1.16. If someone is using just the build output, they won't have any issues even if the version was upgraded to 1.18, however if someone with go version < 1.18 is trying to import the project they won't be able to. |
I've updated the go version in the go.mod in this commit because the changes here will work only for go1.18 or higher. |
Maybe we can use build constraints/build tag, for example: See: |
go.mod
Outdated
@@ -1,5 +1,11 @@ | |||
module github.com/uudashr/gocognit | |||
|
|||
go 1.16 | |||
go 1.18 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Go 1.16 is the minimum required version, I don't think we need to change this to 1.18. Let's make it backward compatible. I've put a comment on the PR.
ref: https://go.dev/doc/modules/gomod-ref
gocognit.go
Outdated
case *ast.IndexListExpr: | ||
targs := make([]string, 0, len(t.Indices)) | ||
for _, exp := range t.Indices { | ||
targs = append(targs, recvString(exp)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are absolutely right. The code works.
Btw, it seems that using assignments is faster (most of the time). What do you think?
see: https://github.com/uudashr/go-bench and try to run
$ go test -benchmem -run="^$" -bench="^BenchmarkSlice_PreAllocated"
goos: darwin
goarch: arm64
pkg: github.com/uudashr/go-bench
BenchmarkSlice_PreAllocated_Assign-8 1000000000 0.0000005 ns/op 0 B/op 0 allocs/op
BenchmarkSlice_PreAllocated_Append-8 1000000000 0.0000010 ns/op 0 B/op 0 allocs/op
PASS
ok github.com/uudashr/go-bench 0.110s
Can we put the test file under the
|
I already setup github actions to run tests on various go versions on every PR. If you want to test it locally try to use
|
recvString func in gocognit returns BADRECV when the function's receiver is a generic type. Implement recvString in two variants in order to keep the code compatible with go versions less than 1.18
Thanks for all of the references and info. I've changed my code based on your feedback. I've tested it with go 1.18 and go 1.16. Regarding the test file - should I create a third test and keep the same structure (to have a separate file in it's own dir e.g. testdata/src/c.go)? |
I think we can create it under
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work. Just need to have unit test for the go1.18 generic
Hi @uudashr , I've added the test in another file with build tag for go1.18. I didn't put the other tests in *pre118_test.go file because they should be executed regardless of the go version. Is that okay ? |
gocognit118_test.go
Outdated
"golang.org/x/tools/go/analysis/analysistest" | ||
) | ||
|
||
func TestAnalyzerWithGenericMethods(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we name it TestAnalyzer_Generic
or something not specific to methods?
The test that we have so far is to test the ability to show the struct/receiver name which has generics declaration. The method itself does not use the generic at all.
So in the future, we can start to add test/capability for gocognit to also works with generic methods and functions.
Ex:
func SumNumbers[K comparable, V Number](m map[K]V) V {
var s V
for _, v := range m {
s += v
}
return s
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.
I was wondering whether to add an example with generic function, but I didn't do it because recvString
only cares about the functions name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed the name to be plural - TestAnalyzer_Generics
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks awesome
HI @ivanruski for such a case package testdata
type Number interface {
int64 | float64
}
func SumNumbers[K comparable, V Number](m map[K]V) V {
var s V
for _, v := range m {
s += v
}
return s
} And run the
Without generic information, we still can identify it clearly and it has the same string representation standard as the generic functions/methods Ex: without generic information
compare to this with generic information
|
Hi @uudashr, At first when I found out that generic types are represented as Giving it a second thought now, I see your point and I think that omitting the generic part of the receivers won't negatively impact the output of the program. On the contrary as you said, we will still be able to identify the method, and the output of the program will be simpler. So I'm onboard with changing the recvString to something like: func recvString(recv ast.Expr) string {
switch t := recv.(type) {
case *ast.Ident:
return t.Name
case *ast.StarExpr:
return "*" + recvString(t.X)
case *ast.IndexExpr:
return recvString(t.X)
case *ast.IndexListExpr:
return recvString(t.X)
}
return "BADRECV"
} I can open another PR if you'd like. |
Another PR would be great. Thanks man |
recvString func in gocognit returns BADRECV when the function's receiver is a
generic type.